-
Notifications
You must be signed in to change notification settings - Fork 745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check-latest flag #141
Add check-latest flag #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
src/distributions/base-installer.ts
Outdated
@@ -24,20 +25,29 @@ export abstract class JavaBase { | |||
)); | |||
this.architecture = installerOptions.architecture; | |||
this.packageType = installerOptions.packageType; | |||
this.checkLatest = installerOptions.checkLatest ?? false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need the ?? false
since getBooleanInput
defaults to it if there is no value
The default should be true, not false. If someone wants to avoid checking for latest update, that may be ok (although I don't see why anyone would choose to do that, ever). But defaulting to that behavior is problematic for many common use cases of setup-java. |
Hello. Thank you for your participation and help with improving setup-java. We prefer to keep check-latest flag as false for default cases, because we want to reduce task duration that is why we prefer to use our image cache as much as possible. Related comment. |
Is there some other place or forum where these discussions and decisions are being made? Who is the team making the decisions? Can I join that team? |
It's important to keep in mind the use-case we're trying to optimize here for. Since late last year our hosted-images come with a few versions of Java from How often are these versions of If a cached version of adopt is used vs downloaded from scratch, the time benefit is ~0s vs ~20s. If So for hosted runners that use For self-hosted runners there is a slightly different use case and I would argue that it makes sense to have That being said,
The core specs for This
For hosted this would clearly benefit us while for self-hosted it behaves just like V1 so IMO characterizing this change as dramatic is a bit of an overstatement |
@konradpabjan I'm confused. Since when has (or had) the pre-installed Java version of the runner anything to do with this action? Does (or did) the action take advantage of the pre-installed Java version, i.e. something like |
This seems like a strange,, distro-specific, image-dependent, and always-stale (by default) way to address the 20 second startup-due-to-no-caching problem. And it does so by overriding [by default] the specified version spec, and providing a stale (multi-months-old) version even when the requested version spec says to use the latest update available. Setup-java has one main role: to set up the java version spelled out in SemVer format in the java-version property. It should not have an "and I really mean it" flag that defaults to "off". Instead, the actual core concern of a 20 second start can be addressed just as well (or to within fraction of a second of) by addressing #84 directly. I would be happy to do the work to address that one, and to commit to build it such that it will support caching for all distros to avoid distro favoritism arguments. Since this is definitely a breaking change being proposed in v2 preview, and a dramatic modification to the common use cases and how OpenJDK updates will flow through to testing. The opportunity to "fix it" with a followup breaking change (that will restore semantics to v1 behavior but provide caching benefits to all distros) will not come up again until v3 [if it is allowed to go into v2 in the current, defaul-is-false proposed behavior]. I therefore strongly object to adding this new property and choosing it's initial default behavior to contradict current v1 behavior. The number of [detectable only during certain days in a year] errors and lapses this will likely cause in the existing ~70K workflows that use the existing semantics will be huge if the default is not set yo be compatible with current behavior.... |
Currently it doesn't take advantage of any cached distributions because our images only cache some adopt distributions while V1 only is structured to pull Zulu distributions: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#java There was a time though when our hosted images cached Zulu distributions but we've switched over, see: actions/runner-images#1057
They do. I wholeheartedly disagree with notion that setup actions should avoid anything with pre-installed versions. There are overwhelming benefits. A good example is the work we did with setup-python v2 that we're now trying to bring over to There are WAY more versions available than the 9 that are cached on each image and you can download those on the fly, the disadvantage though is that downloading and setting up a version dynamically can take a long time (over a minute on Windows for example!). The setup-python action has thus been optimized to use cached distributions when possible for the most possible versions. The cached versions have a slightly slower update cadence but for the majority of users this is fine and there are ways to get the latest and greatest if you're insistent on using the most up-to-date releases. You can read up how setup-java handles using pre-cached versions here: https://github.com/actions/setup-python#available-versions-of-python With setup-java you can imagine someone in V2 using
Solutions such as those outlined in #84 aren't optimal because
The most common asks that we've overwhelmingly seen are people wanting faster builds which is why the default is off. If you're an open source repo or organization that really needs the latest updates of Java as soon as possible always, then you an easily opt-in, but most users that we've seen don't fall into that bucket. There will be documentation added for this like in
I would like to reiterate that characterizing this change as dramatic is a bit of an overstatement. It's a new release, so it's acceptable to make any breaking changes. If you follow our migration guide and you're currently using |
That just states that two random Java versions are installed in the image and that I could run
A cache is something you populate and reuse as long as it isn't outdated, based on a constraint (time, memory, ...). A pre-installed tool on a VM image is NOT a cache. At best you can pre-populate a cache in a VM image.
Python is broken. It needs to be installed and cannot simply be downloaded into a directory and added to $PATH. Please don't use such a broken toolchain as an example.
So is python-versions an indication that there is soon going to be
Of course it should be fast, but then
Yes, hurray, I want faster runs too, especially on Macs! But a) not at the cost of having an arbitrary preinstalled version vs. what comes back from api.adoptopenjdk.net. and b) the time for a run is mostly spent in slow builds, probably due to random disk access, not in I will definitely always set
And there's again the profound confusion between
|
One version is certainly there so that something is in
The most basic definition of caching that i could find online is
Please no 😄 There are enough Java distributions out there and the reason why Python is more complex is because for most platforms you can only download the source code (I'm talking officially from Python.org, you can find a host of ready to install versions but we don't want to use that) and it's up to you to actually compile it (which we do with optimizations)
That's the thing, it's an option and you can customize it to your needs and as long as it's documented then there shouldn't be an issue. Regarding a sensible default, that's when things get very opinionated depending on how you use Java and Actions. We're making the deliberate decision here to optimize and favor certain use cases over others by our choice |
Co-authored-by: Konrad Pabjan <[email protected]>
I'd like to re-iterate that a default value of "false" for check-latest in the proposed v2-preview is a dramatic and unwelcome change in behavior, and one that will be hard to change if v2.0 ends up being released with this surprising default behavior in place. An option to not actually get the latest version that matches your SemVer version spec may make sense, in which case people who want this new but inconsistent behavior (of semi-randomly picking a version based on what happen to be one the image, and of not updating to the latest versions when they become available) in order to gain some temporary speed benefits for some specific distro versions, can choose to turn the option on by saying "check-latest: false". But since doing that this is a clear counter-to-best-practice behavior from a configuration control point of view, it should be something you choose to do explicitly, and definitely not be the default behavior. Let's be clear on somatic meaning here: "check-latest: false" is the semantic equivalent of the following statements: Non of those are a good "default" behavior in a CI/CD environment. Especially so in an echo-system where distros update 4+ times a year, with a well published list of 100s of changes and often 10s of security changes. And yes, "test my repo against the latest released version of Java 11" is a common thing to do, with the intent of finding out if that latest release breaks your stuff. As a practical example, OpenJDK 11.0.9 broke a bunch of stuff that was detected this way, leading to the release of 11.0.9.1. |
Since I can't find any mention of adding locally-pre-installed distros or of overriding the provided SemVer version by default |
* actions/setup-java@v2 - Support different distributions (#132) * Implement support for custom vendors in setup-java * minor improvements * minor refactoring * Add unit tests and e2e tests * Update documentation for setup-java@v2 release * minor improvements * regenerate dist * fix comments * resolve comments * resolve comments * fix tests * Update README.md Co-authored-by: George Adams <[email protected]> * Apply suggestions from code review Co-authored-by: Konrad Pabjan <[email protected]> * fix minor nitpicks * handle 4th digit * pull latest main * Update README.md * rename adoptium to adopt * rename adoptium to adopt * rename adoptium to adopt * Update README.md * make java-version and distribution required for action * update readme * fix tests * fix e2e tests Co-authored-by: George Adams <[email protected]> Co-authored-by: Konrad Pabjan <[email protected]> * Add "overwrite-settings" input parameter (#136) * add overwrite-settings parameter * fix e2e tests * print debug * fix e2e tests * add comment * remove comment * Add "Contents/Home" postfix on macOS if provider creates it (#139) * Update e2e-versions.yml * Update e2e-versions.yml * implement fix * Update e2e-versions.yml * Update installer.ts * fix filter logic * Update e2e-versions.yml * remove extra logic * Update e2e-versions.yml * Add check-latest flag (#141) * add changes for check-latest * run prerelease script * resolving comments * fixing tests * fix spelling * improve core.info messages * run format * run prerelease * change version to fix test * resolve comment for check-latest * Update README.md * added hosted tool cache section * Apply suggestions from code review Co-authored-by: Maxim Lobanov <[email protected]> Co-authored-by: Konrad Pabjan <[email protected]> * Avoid "+" sign in Java path in v2-preview (#145) * try to handle _ versions * more logs * more debug * test 1 * more fixes * fix typo * Update e2e-versions.yml * add unit-tests * remove debug info from tests * debug pre-cached versions * change e2e tests to ubuntu-latest * update npm licenses Co-authored-by: George Adams <[email protected]> Co-authored-by: Konrad Pabjan <[email protected]> Co-authored-by: Dmitry Shibanov <[email protected]>
Description:
In scope of this pull request we add support for check-latest flag.
Related issue: https://github.com/actions/virtual-environments-internal/issues/1942
Check list: